Skip to content

fix: reject LLM calls on a context that cannot serve them - #35

Merged
andinux merged 3 commits into
mainfrom
fix/context-kind-from-options
Aug 31, 2026
Merged

fix: reject LLM calls on a context that cannot serve them#35
andinux merged 3 commits into
mainfrom
fix/context-kind-from-options

Conversation

@andinux

@andinux andinux commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Closes #33.

Supersedes #34, which diagnosed the bug correctly but classified contexts by which llm_context_create_* wrapper was called. That left #33 unfixed on the documented generic form and broke working configurations. This takes the same idea — record what the active context is for, reject operations that cannot work on it — and derives the classification from how the context is actually configured.

Three commits, each independently revertable.

ba55a1c — release the cursor when llm_chat() xOpen fails

llm_chat_cursor_open() allocated the ai_cursor before validating the context and returned SQLITE_ERROR without freeing it. SQLite does not call xClose for a cursor whose xOpen failed, so every rejected SELECT ... FROM llm_chat(...) leaked 48 bytes. Pre-existing; reachable today by querying the vtab with no context created.

46e68be — report failures to the caller being served, not to stale sinks

ai->context / ai->vtab are the error sink for the whole chat and sampler subsystem. About twenty sites report through them; only three ever assigned them, so at nearly every report site the sink was whatever the previous statement left behind.

SELECT llm_context_create_chat('context_size=512');
SELECT llm_chat_respond('hi');       -- publishes this statement's context
SELECT llm_context_free();
SELECT llm_chat_restore('x');        -- reports into the finalized statement

That last line called sqlite3_result_error() on freed memory — SIGSEGV. Without the llm_chat_respond() line the sink is NULL and the same call returned NULL with no error at all. After a vtab scan the message went into vtab->zErrMsg, which SQLite no longer imports: discarded, and the buffer leaked.

Every entry point now publishes its own sink first: the chat scalars, the vtab methods (xFilter and xNext matter as much as xOpen — an interleaved scalar statement can repoint the sink mid-scan), and the 16 llm_sampler_* scalars that reach llm_sampler_check(). llm_chat_disconnect() also left ai->vtab dangling at freed memory, which is what made the second case a use-after-free rather than a lost message.

Pre-existing and unrelated to #33, bundled here because the gate is unreportable without it.

22d3086 — reject generation on a context that cannot produce tokens

Pooled embeddings leave no per-token logits, so the first sampled token reads as EOG and generation returns '' with no error. The chat paths are worse: llm_chat_respond() and the llm_chat() vtab reach llama_sampler_sample(), which dereferences that buffer.

A context is an embedding context when either:

Resolved pooling alone cannot decide this: forcing pooling_type=mean on a generative model still generates correctly. What identifies an embedding model is pooling the caller did not ask for, which llama exposes by resolving LLAMA_POOLING_TYPE_UNSPECIFIED from the model's own hparams. (llama_model_has_decoder() is no use — it returns true for everything except T5ENCODER, BERT included.)

That made "did the caller ask for pooling" load-bearing, which exposed a parser bug: generate_embedding forced pooling_type = MEAN for any value, 0 included. The side effects now apply only when embeddings are actually enabled.

Embedding generation is deliberately not gated — it needs a context that pools, and llm_embed_generate_run() already checks the resolved pooling type, which is a better test than the declared kind.

Known limitation, documented in API.md: passing pooling_type explicitly on an embedding model opts out of the second test and lets generation return '' again. That is the cost of not misreading a caller-forced pooling type on a generative model, which is the more common case.

Compatibility

The gate fires only where generation was already broken — silent '' or a crash — so no working configuration starts failing. Verified across both model families: generic-constructor embedding contexts, caller-forced pooling, chat contexts used with llm_text_generate() (the README vision example), and generate_embedding=0 on a generative model all keep working.

Tests

56 passing, up from 51. Each new test was confirmed to fail against the pre-fix code before being accepted, including a 96-byte leak assertion for the cursor and a SIGSEGV reproduction for the error sink.

Covering the embedding-model half needs an encoder-style model, so the suite now pulls all-MiniLM-L6-v2 (25MB) alongside the existing generative model. That test skips cleanly without --embed-model, so existing checkouts are unaffected.

Follow-ups, not addressed here

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ks738YbgnnJBio2wLtpbAJ

andinux and others added 3 commits August 28, 2026 17:25
llm_chat_cursor_open() allocates the ai_cursor before validating that the
connection has a usable context, then returns SQLITE_ERROR without freeing it.
sqlite does not call xClose for a cursor whose xOpen failed, so the allocation
is lost for the lifetime of the connection - 48 bytes per rejected statement.

Reachable today by querying the vtab with no context created:

    SELECT llm_model_load('model.gguf');
    SELECT reply FROM llm_chat('hi');   -- errors, and leaks

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Ks738YbgnnJBio2wLtpbAJ
…sinks

ai->context and ai->vtab are the error sink for the whole chat and sampler
subsystem. Roughly twenty sites report through
sqlite_common_set_error(ai->context, ai->vtab, ...) - llm_chat_run(),
llm_chat_generate_response(), llm_chat_tokenize_input(), llm_chat_save_response(),
llm_chat_check_context(), llm_sampler_check() - and none of them receives the
caller directly. Only three places ever assigned those fields, so at nearly every
report site the sink was whatever the previous statement happened to leave behind.

Two consequences, both reachable from ordinary SQL:

    SELECT llm_context_create_chat('context_size=512');
    SELECT llm_chat_respond('hi');       -- publishes this statement's context
    SELECT llm_context_free();
    SELECT llm_chat_restore('x');        -- reports into the finalized statement

That last line called sqlite3_result_error() on freed memory: SIGSEGV. Without
the llm_chat_respond() line the sink is still NULL and the same call returned
NULL with no error at all. After a vtab scan the sink is the vtab instead, so
the message went into vtab->zErrMsg, which sqlite only imports immediately after
a vtab method - silently discarded, and the sqlite3_vmprintf buffer leaked.

llm_context_create_with_options() had the same bug from the other direction: it
reported a llama_init_from_model() failure through ai->context/ai->vtab while
every other error in that function used the context parameter it was handed. On
a fresh connection both fields are NULL and the failure vanished entirely, so
the function returned NULL as though it had succeeded.

The rule now: every entry point publishes its own sink before running anything
that can fail. Scalars own a sqlite3_context, vtab methods own the sqlite3_vtab.
Applied at all of them:

  - chat scalars: create, free, save, restore, system_prompt, respond
  - vtab: xConnect, xOpen, xFilter, xNext, xClose. xFilter and xNext matter as
    much as xOpen - both reach llm_chat_run() and llm_chat_generate_response(),
    and an interleaved scalar statement can repoint the sink mid-scan.
  - the 16 llm_sampler_* scalars, which reach llm_sampler_check(). Its
    allocation failure is the one report site with no caller of its own, so
    without this an OOM right after a vtab scan wrote into a vtab->zErrMsg that
    sqlite no longer imports: message lost, buffer leaked.

llm_chat_disconnect() additionally left ai->vtab dangling at freed memory after
the vtab was released, which is what turned the second case above from a lost
message into a use-after-free. The ai_context outlives the vtab, so the
back-pointer has to be cleared.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Ks738YbgnnJBio2wLtpbAJ
Closes #33. llm_text_generate() only checked that a context existed, so calling
it against an embedding context ran on incompatible state. Pooled embeddings
leave no per-token logits, so the first sampled token reads as EOG, the loop
breaks immediately, and the empty buffer is returned as '' with no error. The
chat paths are worse: llm_chat_respond() and the llm_chat() vtab reach
llama_sampler_sample(), which dereferences that buffer.

Track what the active llama context can actually do and reject the operations
that need per-token logits with SQLITE_MISUSE.

The kind is derived from how the context is configured, not from which
llm_context_create_* wrapper was called, because two different things make a
context an embedding context:

  - generate_embedding=1 was passed. llm_context_create() is a general-purpose
    constructor and API.md documents
    llm_context_create('generate_embedding=1,normalize_embedding=1,pooling_type=mean')
    as equivalent to llm_context_create_embedding(), so classifying by call site
    would leave #33 unfixed on the documented generic form.
  - the model pools by default. A BERT-family model (all-MiniLM, nomic-embed)
    carries its pooling type in its own GGUF, so
    llm_context_create('context_size=512,embedding_type=FLOAT32') on one yields
    a pooling context with no embedding settings passed at all - and that is the
    flow API.md recommends for embeddings, so #33 reproduced there too.

Resolved pooling alone cannot decide this: forcing pooling_type=mean on a
generative model still generates correctly. What identifies an embedding model
is pooling the caller did not ask for, which llama makes visible by resolving
LLAMA_POOLING_TYPE_UNSPECIFIED from the model's own hparams. So the test is
ctx_params.embeddings, or an unrequested resolved pooling type.

That makes "did the caller ask for pooling" load-bearing, which exposed a bug in
the option parser: generate_embedding forced pooling_type = MEAN for any value,
0 included. An explicit generate_embedding=0 therefore both configured a pooling
context while asking for embeddings to be off, and marked pooling as
caller-requested - hiding embedding models from the check. The side effects now
apply only when embeddings are actually enabled.

Note llama_model_has_decoder() is no use here - it returns true for everything
except T5ENCODER, BERT included.

Text generation and chat contexts share one kind: llm_context_create_chat() and
llm_context_create_textgen() both pass the same empty option string, so the
contexts are byte-for-byte identical and must stay interchangeable - the README
vision example creates a chat context and then calls llm_text_generate().

Embedding generation is deliberately NOT gated. What it needs is a context that
pools, and llm_embed_generate_run() already checks the resolved pooling type - a
better test than the declared kind, and one that keeps working on every
configuration above.

In the vtab the check is skipped when no context exists at all, so a missing
context still reports the actionable "No context found" rather than a kind
mismatch against LLM_CONTEXT_NONE.

Known limitation, documented in API.md: passing pooling_type explicitly on an
embedding model opts out of the second test and lets generation return '' again.
That is the cost of not misreading a caller-forced pooling type on a generative
model, which is the more common case.

Tests cover both directions on both model families: the rejections, and that a
generic-constructor embedding context, a caller-forced pooling type, and a chat
context all keep working. Covering the embedding-model half needs an
encoder-style model, so the suite now pulls all-MiniLM-L6-v2 (25MB) alongside
the existing generative model; that test skips without --embed-model.

CI never reaches the Makefile's download rule for the other models: the
download-models job fetches them on the host and every build job restores them
from cache. The new model is wired through that same path - URL hash, job
output, host-side restore/download/verify, and a cache restore plus directory in
the build jobs. Without it `make test` fell through to curl, which the Alpine
container used by the linux-musl arm64 jobs does not have (Error 127).

Bumps SQLITE_AI_VERSION to 1.0.8. The workflow reads it via `make version` and
tags from it, so the bump is what makes the release job publish rather than warn
that the version matches the latest release.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Ks738YbgnnJBio2wLtpbAJ
@andinux
andinux force-pushed the fix/context-kind-from-options branch from 3e1d6ff to 06554a2 Compare August 31, 2026 09:05
@andinux
andinux merged commit 7173444 into main Aug 31, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

llm_text_generate() returns '' with no error when the active model/context is an embedding one

1 participant